-
Notifications
You must be signed in to change notification settings - Fork 454
Implement SEP-1577: Sampling With Tools #628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
c2da172 to
f037e01
Compare
|
Hi @alexhancock, could you please take a look at this PR? Thanks! |
|
@DaleSeo Sorry it took me a bit. I'm glad to have support for this, but a little worried about the breaking change. Can you think of any alternatives that would keep compat, or make it a but lighter for updaters? Interested to discuss! |
95e640e to
73e60d7
Compare
|
Thanks for the feedback, @alexhancock! I've made a few updates to simplify the migration:
use std::convert::TryInto;
let content: Content = Content::text("hello");
let sampling_content: SamplingMessageContent = content.try_into()?;
// Before
SamplingMessage { role: Role::User, content: Content::text("hi") }
// After
SamplingMessage::user_text("hi")The type change is unavoidable to support tool use and result content types from SEP-1577, but I think the migration path is now reasonable. Most users can either use the new constructors or add Let me know if you have any other suggestions! |
alexhancock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @DaleSeo, approving but also leaving this list of recommendations.
Generated by Opus 4.5 comparing the diff to https://modelcontextprotocol.io/community/seps/1577--sampling-with-tools.md
## SEP-1577 Implementation Review - Recommended Changes
1. **Missing capability validation (MUST requirement)**: Add runtime validation to throw an error when `tools` or `toolChoice` are provided but `clientCapabilities.sampling.tools` is missing.
2. **Tool use/result balancing not enforced (MUST requirement)**: Add validation that every assistant message with `ToolUseContent` (id: X) is followed by a user message with `ToolResultContent` (toolUseId: X).
3. **Tool result content mixing not enforced**: Spec states "SamplingMessage with tool result content blocks MUST NOT contain other content types" - add validation for this.
4. **Role-content type mismatch possible**: `ToolUseContent` should only appear in assistant messages, `ToolResultContent` only in user messages. Currently not enforced - consider runtime validation.
5. **CreateMessageResult.role not constrained**: Spec says `role: "assistant"` (literal), but implementation allows any `Role`. Minor, but could be tightened.
6. **Dead code**: `AssistantMessageContent` and `UserMessageContent` enums in `content.rs` are defined but never used. Remove or use them.
The validations seem good to do as followups?
|
Thanks for the thorough review, @alexhancock! You're right on all those points. For context on how things ended up this way: I was primarily referencing other SDK implementations and it turns out they don't fully enforce these MUST requirements either, so I carried over the same gaps. I initially planned to use separate Also, thanks for the tip on fetching the SEP markdown directly. I didn't know about that trick and it'll be really useful for future contributions. I've put up a follow-up PR: #646. |
Closes #552
Motivation and Context
This PR implements SEP-1577: Sampling With Tools, enabling MCP servers to run agentic loops using the client's LLM while maintaining user supervision.
Key additions:
ToolChoice/ToolChoiceMode- Control tool selection behaviorToolUseContent/ToolResultContent- Tool calling content typesSamplingContent<T>- Single or array content wrapperSamplingMessageContent- Unified content enum withToolUseandToolResultvariantsSamplingCapability- Structured capability withtoolsandcontextsub-capabilitiesReference implementations:
How Has This Been Tested?
Breaking Changes
The type signature of
SamplingMessage.contentchanged fromContenttoSamplingContent<SamplingMessageContent>.Migration Made Easy
Convenience constructors (recommended):
Converting existing
Contentvalues:Note:
TryFromis used becauseContent::ResourceandContent::ResourceLinkvariants are not supported in sampling messages.Wire Format Compatibility
ClientCapabilities.sampling- Empty{}JSON still deserializes toSamplingCapabilityTypes of changes
Checklist
Additional context